-
Notifications
You must be signed in to change notification settings - Fork 483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ripemd_160
builtin
#6378
ripemd_160
builtin
#6378
Conversation
There were some unexpected changes in the golden files for the |
nix/project.nix
Outdated
@@ -38,6 +38,7 @@ let | |||
|
|||
sha256map = { | |||
"https://github.com/jaccokrijnen/plutus-cert"."e814b9171398cbdfecdc6823067156a7e9fc76a3" = "0srqvx0b819b5crrbsa9hz2fnr50ahqizvvm0wdmyq2bbpk2rka7"; | |||
"https://github.com/paluh/cardano-base"."ea31e27ae4c9715232fa20e2e91f23e5bd967d65" = "sha256-R/uqcScLbkFhYL8x0FL7eQ2UIJY30o6ec312I/E7KdU="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this line? I thought this is only for SRP's and there is no such SRP in the
cabal.project
.
Thanks for pointing those out, @bezirg. I hadn't spotted that. I'll try removing it and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've removed that. Let's see if it makes any difference.
"memory": { | ||
"arguments": 3, | ||
"type": "constant_cost" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VERY IMPORTANT!
This has 2 entries for RIPEMD (with different values actually), which is dangerous considering it is JSON: based on the specific implementation, a json library silently picks the first occurrence or the last occurence. Aeson I think picks the first one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I hope that's fixed now. I added the new costing function but forgot to delete the preliminary one (which was a duplicate of the one for keccak_256). The old one was also in the wrong place, which might not have been so good.
"ripemd_160-cpu-arguments-intercept": 1927926, | ||
"ripemd_160-cpu-arguments-slope": 82523, | ||
"ripemd_160-memory-arguments": 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These numbers do not match the builtinCostModelB&C.json but only one of the 2 duplicates in A.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grrr. I thought I'd been very careful about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That file's only for testing, so I think the actual numbers don't matter. I'll change it anyway, just to be on the safe side.
Somehow this seems to have become broken, possibly due to the changes in #6171. I'll try to fix it later. |
It turned out that a lot of the tests were replicated for the These tests need to be reworked anyway: see #6089. |
* Add ripemd-160 builtin with artificial cost model (copy of keccak_256) * Add conformance tests * More merging * Merge metatheory * Remove ripemd_160 from V3 ParamNames * Update plutus-tx-plugin-tests results * Update cardano-constitution-test results * Add bench results and generate new model for RIPEMD-160 * Update conformance results for new ripemd-160 costs * Add changelog entries * Add changelog entries * Uncomment changleog entries * Fix JSON file * Update defaultCostModelParams.json * Remove entry from sha256_map in project.nix * "Fix" plutus-ledger-api tests * Remove comment about failing test --------- Co-authored-by: Tomasz Rybarczyk <[email protected]>
This updates #6252 to make it mergeable with the current state of
plutus-core
(including all of the bitwise work). This implements the proposed CIP for adding the RIPEMD-160 hash function to Plutus. Thanks to @paluh for doing all the hard work.I've also updated the cost model to include
ripemd_160
. As a precaution I re-ran the costing benchmarks forkeccak_256
to make sure that nothing had changed too much. The attached PNG shows the results. The original results forkeccak_256
are the black points and the new cones s are the blue ones: there's a small difference that's probably due to randomness in our benchmarking machine, but the results aren't dratically different. The results forripemd_160
are the red points and they don't show any unexpected behaviour. The costing function forripemd_160
is a straight line fitted to those points.